Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt AEP 231 - Batch Get #177

Merged
merged 15 commits into from
Oct 25, 2024
Merged

Adopt AEP 231 - Batch Get #177

merged 15 commits into from
Oct 25, 2024

Conversation

mkistler
Copy link
Contributor

Adopt AEP 231 - Batch Get

🍱 Types of changes

What types of changes does your code introduce to AEP? Put an x in the boxes
that apply

  • Enhancement
  • New proposal
  • Migrated from google.aip.dev
  • Chore / Quick Fix

📋 Your checklist for this pull request

Please review the AEP Style and Guidance for
contributing to this repository.

General

Additional checklist for a new AEP

  • A new AEP should be no more than two pages if printed out.
  • Ensure that the PR is editable by maintainers.
  • Ensure that File structure
    guidelines are met.
  • Ensure that
    Document structure
    guidelines are met.

💝 Thank you!

@mkistler mkistler requested a review from a team as a code owner April 22, 2024 14:26
@mkistler mkistler linked an issue Apr 26, 2024 that may be closed by this pull request
aep/general/0231/aep.md.j2 Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Show resolved Hide resolved
@mkistler mkistler requested a review from rofrankel April 27, 2024 14:33
@mkistler
Copy link
Contributor Author

@rofrankel Please re-review.

aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
@mkistler
Copy link
Contributor Author

@rofrankel Thanks for the re-review. I addressed your comments and asked for a new review.

@mkistler
Copy link
Contributor Author

I made the updates we discussed three weeks ago. Please re-review and let me know if there is any additional feedback.

aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
@mkistler
Copy link
Contributor Author

@rofrankel Thanks for the comments. I've addressed or answered all that I could. I will need help on the proto definition of the response.

I will tag you for re-review.

@mkistler mkistler requested a review from rofrankel September 27, 2024 19:31
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the work! The page doesn't seem to render for me today, so I think that change should be made at least.

The rest are pretty minor, and I'll watch this PR closely on replies so we can resolve them quickly. Thank you!

aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0231/aep.md.j2 Show resolved Hide resolved
@mkistler
Copy link
Contributor Author

Thank you for the review and suggestions @toumorokoshi. I accepted all these. Is there anything else to do or can we merge this now?

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work! approved.

@toumorokoshi toumorokoshi dismissed rofrankel’s stale review October 25, 2024 16:04

@toumorokoshi is taking over the review

@toumorokoshi toumorokoshi merged commit 39e5180 into aep-dev:main Oct 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt AIP-0231 Batch methods: Get
4 participants